Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Expose Vector* component-wise and scalar min/max to scripting #80223

Merged
merged 1 commit into from
May 2, 2024
Merged

Expose Vector* component-wise and scalar min/max to scripting #80223

merged 1 commit into from
May 2, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Aug 3, 2023 •
edited
Loading

Added for convenience, which was mentioned in the PR changing the behavior of the generic min/max

If the naming is considered confusing we can use component_max/min just to make it more clear (though more verbose)

Will open a godot-cpp equivalent as well
Done: godotengine/godot-cpp#1197

@AThousandShips AThousandShips added this to the 4.x milestone Aug 3, 2023
@AThousandShips AThousandShips requested review from a team as code owners August 3, 2023 14:19
@AThousandShips AThousandShips requested a review from a team as a code owner August 3, 2023 15:22
@MewPurPur
Copy link
Contributor

Regarding the resolved comment above, shouldn't documentation examples, on the contrary, avoid static typing? So no mini() and minf() for example?

@AThousandShips
Copy link
Member Author

AThousandShips commented Aug 3, 2023 •
edited
Loading

Here the typing is static because of the vector types, no, and the specific versions have better performance so would recommend using them whenever reasonable? But can switch back if desired

@AThousandShips AThousandShips mentioned this pull request Aug 3, 2023
Merged
@AThousandShips AThousandShips removed the request for review from a team November 5, 2023 19:03
@AThousandShips
Copy link
Member Author

Added note to the documentation in line with #84656

@raulsntos
Copy link
Member

Unfortunately the Min/Max ones don't fit in the integer vector versions because they already have Min/Max as constants

Luckily those members have been added in 4.2 (#81819) which is still unreleased so we have a chance to rename them. I would name them MinValue and MaxValue to match the int names. Then we can use the Min and Max names for the methods which seems consistent with the methods in Mathf.

@AThousandShips
Copy link
Member Author

Great! Was considering that, will make the changes here and see what ends up happening

@AThousandShips
Copy link
Member Author

AThousandShips commented Nov 9, 2023 •
edited
Loading

Resolved it for the CI here, but would be maybe good to get a dedicated one for C# done for 4.2 anyway

I'll go ahead and make one

@AThousandShips AThousandShips mentioned this pull request Nov 9, 2023
Merged
@AThousandShips AThousandShips mentioned this pull request Mar 3, 2024
Merged
@AThousandShips AThousandShips force-pushed the vec_elem branch 3 times, most recently from 7215ad9 to bdb56f3 Compare March 4, 2024 15:29
@AThousandShips

This comment was marked as outdated.

Sign in to view
@AThousandShips AThousandShips marked this pull request as draft May 2, 2024 08:30
@AThousandShips AThousandShips changed the title Expose Vector* component-wise min/max to scripting Expose Vector* component-wise min/max and scalar mini/maxi to scripting May 2, 2024
@AThousandShips AThousandShips changed the title Expose Vector* component-wise min/max and scalar mini/maxi to scripting Expose Vector* component-wise and scalar min/max to scripting May 2, 2024
@AThousandShips AThousandShips marked this pull request as ready for review May 2, 2024 11:03
@akien-mga akien-mga modified the milestones: 4.x, 4.3 May 2, 2024
@akien-mga akien-mga merged commit 396ce1a into godotengine:master May 2, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the vec_elem branch May 2, 2024 12:05
@AThousandShips
Copy link
Member Author

Thank you!

@AThousandShips AThousandShips mentioned this pull request Jun 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga approved these changes

@reduz reduz Awaiting requested review from reduz

Assignees
No one assigned
Labels
enhancement topic:core
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

Expose Vector* component-wise min/max to scripting
4 participants
@AThousandShips @MewPurPur @raulsntos @akien-mga